-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Support Field Revalidation #9
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tyObserver` Although this is a breaking change, it will more easily allow us to support the `revalidateOn` feature that we mentioned on GitHub while also maintaining the parent-child relationship between the `FormObserver` and the `FormValidityObserver`. (We could technically choose to break that relationship, but the reusable `observe` and `unobserve` methods seem to be proving helpful right now.) This change also seems more logical because -- practically speaking -- developers are likely going to choose one event anyway. They'll have to choose an event that actually relates to user interaction. And the most common of those events are `input`, `focusout`, and `change`. However, the final `input` triggers before `focusout` happens, so it doesn't make sense to specify both simultaneously. Similarly, the `change` event will only be relevant for a subset of the times that the `focusout` event is triggered, so supplying those two simultaneously also doesn't make sense. So developers will likely only supply 1 event at a time.
This is not all of the ESLint packages. We're only updating a few. The `@typescript-eslint` packages are of particular interest since our previous version did not support `[email protected]`.
This "Manual Mode" can be enabled by passing `null` to the `type` argument of the constructor. The main reason that this is useful is that it gives developers a way to only validate their forms `onsubmit` if they so please.
This feature will be especially helpful for developers who only want to validate their fields `oninput` _after_ their form has already been submitted.
We forgot about these limitations... And because we forgot about these limitations, we ran off and tried to remove the `FormStorageObserver.d.ts` and `FormValidityObserver.d.ts` files... But then we saw the problems that came up when TypeScript attempted to generate these files automatically from the JS files... We're not interested in enduring that headache again. Note that even if we remove generic constructors from the `FormStorageObserver` and the `FormValidityObserver`, TypeScript will still get confused. We've further documented why we're stuck in this predicament, and we hope the TypeScript team will pull through for us at some point.
Includes a potential feature idea for the `FormValidityObserver`. This feature would be easy to support and is likely not a super high priority (unless users make it one).
We missed some of these documentation updates/improvements while updating our project. Hopefully the docs are better now, though surely there are other improvements that could be made.
Originally, this hook seemed like a good idea. But actually, it's turned out to be something less performant than expected, and something that's more prone to footgunning than expected. See the `Design Decisions` document for additional details. Notice that `react` is still a peer dependency of `@form-observer/react`. In the future, we might use the React package to spy on the React version so that we can figure out how to appropriately generate the `autoObserve` function. If we decide against that, we can remove the peer dependency. Again, see the `Design Decisions` document for additional details.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We're finally adding support for field revalidation, as it seems like this is an important feature to have in form validation tools. Closes #8.
Highlights:
FormValidityObserver
'stype
constructor argument.null
to thetype
argument of theFormValidityObserver
to use it in "Manual Mode".revalidateOn
option.useFormValidityObserver
React hook is being removed because it isn't genuinely useful.The "Manual Mode" feature and the revalidation feature can be combined to allow developers to validate form fields
oninput
, but only after a form submission has occurred.We're excited about these changes and eager to see how things play out from here!
Additional Details
Below are more details on some of the significant design decisions that we made. You can also find these details in our
DESIGN_DECISIONS
markdown file.Only Allow One (1) Event Type to Be Passed to the
FormValidityObserver
ConstructorAlthough this is a breaking change, it is one that will more easily allow us to support the
revalidateOn
feature that we mentioned on GitHub while also maintaining the parent-child relationship between theFormObserver
and theFormValidityObserver
. (We could technically choose to break that relationship, but the reusableobserve
andunobserve
methods seem to be proving helpful right now.)It technically isn't that difficult to continue supporting an array of strings for the
types
argument of theFormValidityObserver
constructor. But the (very small) additional effort for this support did not seem worth it.Practically speaking, most (if not all) developers are likely going to choose only one event type anyway. And they'll have to choose one of the events that actually relate to user interaction (so that the form is only validated as users interact with it). The most common of these events are
input
,focusout
(the bubbling version of theblur
event), andchange
. But each of these events renders the other ones irrelevant.For example, the last
input
event that's triggered for a form field will always be triggered beforechange
orfocusout
. Consequently, it doesn't make sense to addchange
orfocusout
to thetypes
argument of theFormValidityObserver
constructor wheninput
is supplied. The developer would only need to specify theinput
event itself.Similarly, the
change
event is only fired a subset of the times that thefocusout
event is fired. (There are technically exceptions to this rule, such as<select>
and<input type="checkbox">
. But practically speaking, this rule holds for all form controls -- including those.) So instead of specifying both events, a developer would only need to specify thefocusout
event. Finally, if a developer wants to respond tochange
events exclusively, then they will only specify that event. Neitherfocusout
norinput
could be added in that scenario because it would cause the form controls to be validated too frequently.In the end, whichever event type is chosen, it is only practical for the developer to specify 1 event. Technically speaking, developers could try to specify custom events. But those events would likely be emitted in response to
input
/focusout
/change
events anyway. So everything ultimately comes down to those 3 events.In conclusion, it will be simpler for us to only allow 1 event to be specified for the
types
argument moving forward. Since this is also what will happen naturally (and since enforcing this might protect some developers from unexpected footguns), this seems like a good change.Deprecate the
useFormValidityObserver
React HookOriginally, we thought that it would be a good idea to memoize the
FormValidityObserver
for the React developers who would use our tool. That was the idea behind ouruseFormValidityObserver
hook: It would enable developers to use theFormValidityObserver
without having to think about memoization. It's a great idea! And when possible, I think that maintainers should take this route! But unfortunately, this approach is not always an option...Consider a scenario where a developer uses the
useFormValidityObserver
hook with a complex configuration:The result returned by
useFormValidityObserver
is memoized. However, theuseMemo
hook (which the function calls internally) uses the function'stype
argument and object options as its dependencies. If theMyForm
component re-renders for any reason, the"focusout"
and"input"
strings will remain constant. But therenderer
function and thedefaultErrors
object will be re-defined during re-renders, causing theuseMemo
hook to create a newFormValidityObserver
. So those options need to be memoized.As you can see, this code becomes very ugly very quickly. Even worse, the code becomes less performant. Why? Well, because memoization comes with a cost. And the more that memoization is used, the more costs users will have to pay. Yes, memoization is helpful. Yes, under the right circumstances, memoization can bring performance benefits. But it should be used as little as possible and only where it's truly needed.
Here in our example, we're performing 3 memoizations for a single function call. (One for
defaultErrors
, one forrenderer
, and one foruseFormValidityObserver
's internal call touseMemo
.) That's not good. A better solution is to memoize the entire result once. But wait... Couldn't we just do that withcreateFormValidityObserver
?In fact, this is more performant than
useFormValidityObserver
! If we memoize the result fromuseFormValidityObserver
, then we perform 2 memoizations. (One foruseFormValidityObserver
's internal call touseMemo
, and one for our own call touseMemo
on the function's result). But if we simply memoize the result ofcreateFormValidityObserver
directly, then we only have to perform 1 memoization.What does all of this mean? Well... It means that by default, even in the most ideal scenario,
useFormValidityObserver
will never be more performant than callinguseMemo
oncreateFormValidityObserver
directly. And in most cases,useFormValidityObserver
will be less performant. Moreover, many developers won't even be aware of the concerns that we've discussed so far; so they're much more likely to footgun themselves if they use the flashyuseFormValidityObserver
hook!We wanted to find a way to protect developers from
useMemo
, but we couldn't. (And honestly, no developer who wants to be skilled with React can avoid learning aboutuseMemo
in this day and age -- not until React Forget stabilizes, at least.) This dilemma is just an unavoidable downside of how React decides to operate as a framework, and we sadly can't do anything to fix that. What we can do is guide developers on how to usecreateFormValidityObserver
in React easily and performantly. And that's exactly what we've decided to do instead: Add clearer documentation as a better alternative.Thankfully, things really aren't that complicated. People just need to wrap
createFormValidityObserver
insideuseMemo
(when using functional components). That's it.Sidenote: Although we're removing the
useFormValidityObserver
hook, we aren't removing React as a peer dependency [yet]. The reason for this is that we're trying to figure out how to prepare for React 19's changes to ref callbacks (for the sake ofautoObserve
), and we're debating using the React package to help us with this. For example, we could use the React package to tell us the React version being used. Then, we could return one kind ofautoObserve
function if the React version is less than 19, and return a different kind ofautoObserve
function if the React version is greater than or equal to 19. But maybe that's a bad idea? Maybe we should just maintain different versions of our@form-observer/react
package? Then again, it might not be a bad idea either since it seems likeimport { version } from "react"
is permitted (at least as of today). We'll see...